Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update IDP signin status API explainer #505

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

cbiesinger
Copy link
Collaborator

Per discussion at TPAC.

@cbiesinger
Copy link
Collaborator Author

@samuelgoto @martinthomson @bvandersloot-mozilla -- could you sanitycheck that this represents what we discussed?

@cbiesinger
Copy link
Collaborator Author

Also should I rename this to "IDP Login Status API" now, since we changed signin to login in the API?

aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 20, 2023
As described in https://github.com/fedidcg/login-status and
w3c-fedid/FedCM#505

[email protected]

Bug: 1451396
Change-Id: I10b4a7b93e72184f86d5116b2708d3918a68573c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4874093
Reviewed-by: Zachary Tan <[email protected]>
Commit-Queue: Zachary Tan <[email protected]>
Auto-Submit: Christian Biesinger <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1199147}
Copy link
Collaborator

@npm1 npm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to keep this updated, I think there are some more changes needed:

  • A few places have "signed-in" or "signed-out" where it should use "logged-in" or "logged-out"
  • Should the state naming also change?
  • The alternatives considered seciton also needs to be updated. In particular, it still mentions "action=signin".

proposals/idp-sign-in-status-api.md Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
@samuelgoto
Copy link
Collaborator

Just wondering, since there is a lot of duplication between the two explainer, why wouldn't we just point people to this explainer instead [1] for the specifics, and this explainer can go over how FedCM intends to use the signal ?

https://github.com/fedidcg/login-status

Copy link
Collaborator Author

@cbiesinger cbiesinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed the comments.

I added a link to the login status explainer but wanted to keep the two sections in this document so readers don't need to read two documents to understand the proposal.

proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
proposals/idp-sign-in-status-api.md Outdated Show resolved Hide resolved
@samuelgoto samuelgoto merged commit aafc66f into w3c-fedid:main Sep 29, 2023
2 checks passed
@cbiesinger cbiesinger deleted the explainer2 branch September 29, 2023 17:08
github-actions bot added a commit that referenced this pull request Sep 29, 2023
SHA: aafc66f
Reason: push, by samuelgoto

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to mattdanielbrown/WebID that referenced this pull request Sep 30, 2023
SHA: aafc66f
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
npm1 pushed a commit that referenced this pull request Oct 4, 2023
* Update IDP signin status API explainer

Per discussion at TPAC.

* restrict to secure context

* s/signed/logged/g

* remove action

* ted comments

* better describe parsing of the header

* review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants